Skip to content

feat(validation): capture scoped HTTP authorization - #126

Draft
pshickeydev wants to merge 1 commit into
developmentfrom
feat/safe-exec-development
Draft

pshickeydev wants to merge 1 commit into
developmentfrom
feat/safe-exec-development

Conversation

@pshickeydev

@pshickeydev pshickeydev commented Nov 7, 2026 •

Copy link
Copy Markdown
Member

Summary

  • Extend validation plans with structured HTTP probe fields and reject command or rollback text on those probes.
  • Preserve exact HTTP targets, cluster API and discovery policy, and timestamp expiry in authorization snapshots while retaining compatibility with existing snapshots.
  • Register the new HTTP vocabularies and test structural and boundary validation.

Verification

  • Contract push gates: 1,510 passed, 42 skipped; PostgreSQL was unavailable.
  • Built and installed the wheel; confirmed its packaged schemas and five HTTP vocabularies are present.

Coordinated review

Draft pending coordinated review and integration. Core's temporary contracts pin must be reconciled with the remediation cutover #391 before Core merges.

This pull request description was generated with AI assistance.

@openshift-ci openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 7, 2026
@openshift-ci

openshift-ci Bot commented Nov 7, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pshickeydev

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 7, 2026
@pshickeydev

Copy link
Copy Markdown
Member Author

/agentic_review

@qodo-code-review

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. Plans without HTTP destinations pass validation 🐞 Bug ≡ Correctness
Description
http_probe makes mode and every destination selector optional, so a step with target.http: {}
passes the plan schema. The repository's artifact validator has no additional plan gate, allowing an
endpoint-less probe into an accepted plan even though execution would need a destination to resolve.
Code

artifacts_schema/v1/validation-plan.schema.json[R80-84]

+    "http_probe": {
+      "type": "object",
+      "additionalProperties": false,
+      "properties": {
+        "mode": {"enum": ["route", "service", "node", "direct"]},
Evidence
The new probe definition has no required constraint and permits an empty object; the CLI runs
schema validation without a plan-specific content gate. The added positive test supplies mode and
selectors, but does not test their absence. This is a contract-validation finding, not a claim that
an executor dispatches the invalid probe.

artifacts_schema/v1/validation-plan.schema.json[80-96]
validate.py[47-77]
tests/test_validation_plan.py[87-113]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The plan schema accepts an empty HTTP probe with no endpoint-selection mode or destination, despite treating it as a structured HTTP step.
## Fix Focus Areas
- artifacts_schema/v1/validation-plan.schema.json[80-96]
- tests/test_validation_plan.py[112-137]
## Recommended Fix
Require the fields needed to identify and execute a probe, including its mode, and add mode-specific destination requirements consistent with the supported selectors. Add negative tests for an empty probe and destination-less modes while retaining the existing valid route example.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Dismiss ↗ | View ↗


2. Incomplete HTTP scope snapshots pass validation 🐞 Bug ≡ Correctness
Description
http_discovery has no required policy fields, and its node grant likewise accepts an empty object,
so a cluster can carry http_discovery: {"nodes": {}} without recording selectors or credential and
TLS decisions. When such an object is submitted as an authorization snapshot, the artifact validator
accepts it as structurally complete without a separate authorization content gate; no downstream
interpretation of the omissions is established here.
Code

artifacts_schema/v1/validation-authorization.schema.json[R98-103]

+    "http_discovery": {
+      "type": "object",
+      "additionalProperties": false,
+      "properties": {
+        "routes": {"type": "array", "items": {"$ref": "#/$defs/http_route_grant"}},
+        "nodes": {"oneOf": [{"$ref": "#/$defs/http_node_grant"}, {"type": "null"}]},
Evidence
The discovery and node definitions have no required lists, and route grants require only a
namespace. The README describes these values as part of the approved scope snapshot; the validator
adds no authorization-specific content checks. This establishes acceptance of incomplete records,
not an access-control bypass in an external executor.

artifacts_schema/v1/validation-authorization.schema.json[98-130]
README.md[235-240]
validate.py[76-77]
validate.py[84-109]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A present HTTP discovery policy and its nested node or route grants can omit the fields needed to record the approved policy, while the authorization artifact still validates.
## Fix Focus Areas
- artifacts_schema/v1/validation-authorization.schema.json[98-130]
- tests/test_validation_authorization.py[82-120]
## Recommended Fix
Keep the outer discovery field optional for older snapshots, but require the policy fields that must be preserved when a discovery object is present. Apply corresponding requirements to node and route grant objects, and add negative tests for incomplete objects alongside the existing complete-snapshot tests.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Dismiss ↗ | View ↗


Grey Divider

Context sources
✅ Linked PRs:
  🌐 PR #79
❌ Tickets: failed to load — Ticket collection could not establish complete evidence
Review mode: Auto: ⚖️ Balanced: Authorization snapshot and schema contract changes are high-risk, though largely declarative.

Grey Divider

Tip of the day
💡 Did you know, you can hide the parts of a finding you never read, like the evidence or the agent prompt

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

@pshickeydev

Copy link
Copy Markdown
Member Author

Qodo follow-up: 99ad6af requires complete fields in present HTTP discovery policies and route/node grants, and requires a nonempty URL for direct probes. Older snapshots may still omit the discovery policy. Verified 98 focused contract tests and exercised the validator CLI with accepted and rejected examples.

Route/service plans can still contain empty namespace candidates because the current planner emits them; the contract describes a proposed plan, and authorization/resolution rejects non-executable probes. An omitted mode retains the consumer's route default. Tightening these remaining shapes needs coordinated planner/contract changes rather than silently invalidating generated plans.

This comment was generated with AI assistance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant